Implement JsonSerializable for SplFixedArray #7117
Conversation
| @@ -49,4 +49,6 @@ public function offsetSet($index, mixed $value) {} | |||
| public function offsetUnset($index) {} | |||
|
|
|||
| public function getIterator(): Iterator {} | |||
|
|
|||
| public function jsonSerialize(): array {} | |||
There was a problem hiding this comment.
This is I think more a question of convention. The JsonSerializable interface defines jsonSerialize() as having a mixed return type. Even though this implementation does only ever return an array, will it cause issues if someone wanted to extend SplFixedArray and override jsonSerialize() with a different return type like JsonSerializable?
I can't think of many situations where there would be a reason someone would have to do this, but I think if they were to be using this stub for intelisense it might complain that the child's method is incompatible with SplFixedArrays even though it should technically be allowed by the interface.
This is the deepest I've ever seen into PHPs internals so I don't know how else the stubs are used and whether running that hypothetical example would actually generate any errors.
There was a problem hiding this comment.
Yes, a child class will not be allowed to widen the type. I guess if we really want we can declare this as a tentative type instead, so that any code hypothetically extending SplFixedArray with a different return type would not be immediately broken.
cc @kocsismate
There was a problem hiding this comment.
Interesting problem.. Personally, I don't have many too much concern about the array return type (I'm not sure how common is to extend SplFixedArray + implement JsonSerializable), but if we want to be extra safe, a tentative return type seems like a good idea.
There was a problem hiding this comment.
For what it is worth, I am not overtly concerned either. I just saw it and wasn't sure if it was an overt decision to deviate from the interface.
There was a problem hiding this comment.
I think I'll go with the explicit type here and change it to a tentative one if anyone complains that it broke their code :) It seems pretty unlikely to me.
|
nit: Should this also update ext/spl/config.m4 and config.w32 to note the dependency on I'm not familiar with the reasons to include |
|
@TysonAndre I tried to figure out what PHP_ADD_EXTENSION_DEP would do for always required extensions. The module deps are important because they determine module startup order. Apparently PHP_ADD_EXTENSION_DEP should theoretically affect module registration order via internal_functions.c. It doesn't look like that actually works though (e.g. standard occurs after spl there), and I'm not sure under what circumstances registration order would matter either. |
|
I tracked down what the issue is: php-src/build/order_by_dep.awk Line 14 in 5399489 $1. If it's not present all arguments get shifted by one and we get a nonsense dependency.
This is what happens when you try to program in unix tools. |
This returns an array for SplFixedArray JSON encoding, which is more appropriate than an object with integer string keys. Implements request https://bugs.php.net/bug.php?id=81112.
nikic
force-pushed
the
spl-fixedarray-json
branch
from
225886c to
056fe2b
Compare
|
I fixed the ordering script in be92a87 and added the json build dependency here, though I doubt it has any effect. |
(since ImmutableKeyValueSequence implements JsonSerializable) Source: php#7117 by nikic
This returns an array for SplFixedArray JSON encoding, which
is more appropriate than an object with integer string keys.
Implements request https://bugs.php.net/bug.php?id=81112.